Skip to content

Add centered pressure gradient#353

Merged
mark-petersen merged 58 commits intoE3SM-Project:developfrom
sbrus89:omega/pgrad
Apr 6, 2026
Merged

Add centered pressure gradient#353
mark-petersen merged 58 commits intoE3SM-Project:developfrom
sbrus89:omega/pgrad

Conversation

@sbrus89
Copy link
Copy Markdown
Collaborator

@sbrus89 sbrus89 commented Feb 24, 2026

This PR adds a centered, second-order accurate, horizontal pressure gradient tendency term to Omega.

Checklist

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Feb 24, 2026

Testing
Passes all CTests on pm-cpu and pm-gpu, except for IO_TEST. I'm not sure why this test would be affected by this PR.

Passes Polaris ocean/horiz_press_grad/salinity_gradient, ocean/horiz_press_grad/temperature_gradient, and ocean/horiz_press_grad/ztilde_gradient. Convergence plots below:

ocean/horiz_press_grad/salinity_gradient
omega_vs_reference

ocean/horiz_press_grad/temperature_gradient
omega_vs_reference

ocean/horiz_press_grad/ztilde_gradient
omega_vs_reference

Omega pressure gradient has 10^-16 - 10^-13 differences with the python implementation in polaris.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Feb 24, 2026

Polaris omega_pr suite

  • Baseline workdir: /pscratch/sd/s/sbrus/polaris_pgrad_pr_suite_baseline
  • Baseline build: /global/homes/s/sbrus/scratch/polaris_pgrad_pr_suite_baseline/build
  • PR build: /global/homes/s/sbrus/scratch/polaris_pgrad_pr_suite/build
  • PR workdir: /global/homes/s/sbrus/scratch/polaris_pgrad_pr_suite
  • Machine: pm-cpu
  • Compiler: gnu
  • Build type: Release
  • Log: not found
  • Result:
    • Failures (2 of 9):
      • ocean/spherical/qu/rotation_2d
      • ocean/spherical/icos/cosine_bell/restart
    • Diffs (1 of 9):
      • ocean/horiz_press_grad/salinity_gradient

ocean/spherical/qu/rotation_2d also failed in baseline.
I'm looking into ocean/spherical/icos/cosine_bell/restart fail.

Copy link
Copy Markdown

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on @sbrus89 and my testing of the code in Polaris (both based on a python implementation of the Omega algorithm and a high-fidelity reference solution produced by vertical numerical quadrature and 4th-order horizontal gradients), and a careful inspection of the code changes.

@sbrus89 sbrus89 requested a review from brian-oneill March 4, 2026 16:13
@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 4, 2026

The IOSTREAM_TEST only fails on a re-run when the test/ocn.*.nc files are already present. I noticed this also happens in the develop branch.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 4, 2026

The ocean/spherical/icos/cosine_bell/restart polaris test fail was fixed by @cbegeman's suggestion to turn off the pressure gradient tendency. I will submit a follow-on polaris PR that does that for cases that do not require a pressure gradient term.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 5, 2026

The IOTest fail was an issue on my end with having an out-of-date scorpio submodule.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 5, 2026

This now passes CTests with gnu Release/Debug on pm-cpu and gnugpu Release/Debug on pm-gpu.

@sbrus89 sbrus89 requested a review from mark-petersen March 20, 2026 21:32
Copy link
Copy Markdown
Member

@mwarusz mwarusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more suggestions.

VCoord->computePressure(LayerThick, SurfacePressure);
OMEGA_SCOPE(LocPressureMid, VCoord->PressureMid);
Array2DReal Temp = Tracers::getByName(VelTimeLevel, "Temperature");
Array2DReal Salinity = Tracers::getByName(VelTimeLevel, "Salinity");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using VelTimeLevel doesn't seem right here too, but it is a bit harder to fix. One option is to make computeVelocityTendenciesOnly and computeVelocityTendencies take in the same TracerArray that gets passed to computeAllTendencies. Another one is to add TracerTimeLevel everywhere.

Copy link
Copy Markdown

@brian-oneill brian-oneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. Ran the ctests successfully on pm-cpu and pm-gpu. Just a couple things below, nothing major.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 26, 2026

Thanks for the feedback @mwarusz and @brian-oneill. I think I've addressed your comments in the the last couple commits.

@mwarusz, I choose to add TracerTimeLevel to the computeVelocityTendenciesOnly, computeVelocityTendencies, and computeAllTendencies. Let me know if you think that works okay or if you'd like to see changes.

Related to the TracerTimeLevel change is where we actually want to call computeSpecVol. I'm open to moving it to another location.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 27, 2026

This passes CTests on pm-gpu and pm-cpu as well as the omega_pr suite on pm-cpu.

@mwarusz
Copy link
Copy Markdown
Member

mwarusz commented Mar 27, 2026

@sbrus89

Oops, turns out that using TracerTimeLevel is not really doing the right thing in RungeKutta4 currently. I'm sorry for suggesting that change. The reason why it is not correct is because these calls

Array2DReal Temp     = Tracers::getByName(TracerTimeLevel, "Temperature");
Array2DReal Salinity = Tracers::getByName(TracerTimeLevel, "Salinity");

get temperature and salinity from the tracers array in the singleton Tracers instance. However, intermediate RK4 stages should get these tracers from the ProvisTracers array.

In general, I believe that we should have TracerTimeLevel. However, in order to make it work, we would need to refactor the Tracers class to not be a singleton. Then ProvisTracers could become a proper Tracers class instance, instead of a plain array.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 30, 2026

@mwarusz , that makes sense with respect to RK4. I'm open to making Tracers a non-singleton class, but I suppose we could also pass the 3D Tracer array (either from Tracers::getAll() or the ProvisTracers array) and use IndxTemp and IndxSalt to access the right subviews. But that essentially duplicates what the Tracers::getByIndex() function does, so maybe it's better to go the non-singleton route.

@sbrus89
Copy link
Copy Markdown
Collaborator Author

sbrus89 commented Mar 31, 2026

@mwarusz, I am now passing a tracer array into computeVelocityTendenciesOnly(). I left the TracerTimeLevel argument even though it isn't serving a functional purpose at this point. I can remove it or leave it for now until we switch Tracers to a non-singleton class.

@mark-petersen
Copy link
Copy Markdown
Collaborator

Tested head on perlmutter gnu CPU, GPU, and passes all cTests.

I tested the baroclinic channel using polaris_0.10.0-alpha.1 and the setup

polaris setup -t  ocean/planar/baroclinic_channel/1km/default --model mpas-ocean -p $e/master/components/mpas-ocean -w $r/260401-BclCh-MpAB2

etc for both MPAS-O and Omega, for resolutions 10km, 4km, 1km. All use the default namelist except I set horizontal viscosity (del2) to 20 m^2/s^2 to match the Petersen et al. 2015 paper.

Omega simulations generally have the right behavior, but with some notable differences. Here is

  • left: Omega 1km RK4, dt=2s nu=20
  • right: MPAS. 1km AB2, dt=30s nu=20

https://github.com/user-attachments/assets/c19c2d18-381e-46cc-9b8e-a021dc6f1588
Video might not work. Here are snapshots:
Day 3: Something is wrong on the temperature gradients early on. Perhaps horizontal advection is anti-diffusive, or vertical advection is moving water up the column?
image

Day 4.5: Structures look different
image

Day 7: Waves visible in Omega (left)
image

Copy link
Copy Markdown
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously, we have some work to do. However, I think we should go ahead and merge this PR and then continue testing on develop. The convergence tests for the pressure gradient term, above, show that this term is working as designed. The issues in my last post are likely due to other terms, or the interaction of terms that passed their individual tests.

@mark-petersen
Copy link
Copy Markdown
Collaborator

mark-petersen commented Apr 2, 2026

Here is some initial timing information. This is not specific to this PR, but I thought I would post it here because the pressure gradient allows us to collect timing data for primitive equation tests for the first time. Using the branch hyungyukang/hkang/omega/new-test-branch with this PR fully merged in.

Summary of Omega vs MPAS-O timing with primitive equation terms.
Test is 1km baroclinic channel with 92k cells on 4 perlmutter nodes, 512 CPUs. Timing averaged over 10 RK4 timesteps. Timing includes the first time step for both models. The first time step is often 50% slower.

Timing without i/o:
Omega: 0.02658s/timestep (3% slower)
MPASO: 0.02579s/timestep

Full timing, with i/o, init, finalize for 10 RK4 timesteps:
Omega: 10.273s (2.3% slower)
MPASO: 10.041s

@mark-petersen
Copy link
Copy Markdown
Collaborator

For reference, unsplit-RK4 dt=5s (left) versus split-AB2 dt=30s (right), both in MPAS-O, produce negligible differences. This is 1km resolution, day 4.
image

Copy link
Copy Markdown
Member

@mwarusz mwarusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments @sbrus89. This looks good now. I am approving, but note that either #378 or #375 should be merged first to fix #367.

Copy link
Copy Markdown

@hyungyukang hyungyukang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR after merging it into the time-stepping working branch (https://github.com/hyungyukang/Omega/tree/hkang/omega/new-test-branch).

Using that branch, I ran the Omega horizontal pressure gradient test in Polaris (E3SM-Project/polaris#465) and got the same convergence rate reported by @sbrus89 above:

Image

@hyungyukang
Copy link
Copy Markdown

hyungyukang commented Apr 5, 2026

As @mark-petersen noted, I have also observed somewhat different behavior between Omega and MPAS-O in the baroclinic channel test at 4 km resolution.

Below are my results from the 4 km baroclinic channel test, along with a comparison between Omega and MPAS-Ocean. Waves appear in Omega by day 20, similar to what @mark-petersen reported at 1 km resolution.
image

The time series of the domain-integrated kinetic energy:
image
Omega and MPAS-O begin to diverge after about day 5, and Omega shows slightly higher total KE.

@xylar
Copy link
Copy Markdown

xylar commented Apr 5, 2026

@mark-petersen and @hyungyukang, if you said, U missed it but are you using a second order pressure gradient in MPAS-O as well in your tests?

@hyungyukang
Copy link
Copy Markdown

@mark-petersen and @hyungyukang, if you said, U missed it but are you using a second order pressure gradient in MPAS-O as well in your tests?

@xylar , I’ve been using pressure_and_zmid in MPAS-O, and I believe @mark-petersen has been doing the same (Please correct me if you’re using a different one.).

@xylar
Copy link
Copy Markdown

xylar commented Apr 5, 2026

Okay, that's likely the closest equivalent. Sounds right.

@hyungyukang
Copy link
Copy Markdown

I ran experiments with real bottom topography at QU240km resolution without forcing (kind of a spin-down configuration).

Omega blows up within 4 hours, producing NaN values in the variables, whereas MPAS-O remains stable through 20 days. When the PGF term is disabled, although this is not a physically appropriate configuration, Omega also remains stable through 20 days. The instability in Omega may be caused either by its interaction with other terms or by the PGF term itself, and this will need to be investigated further.

FYI, the domain integrated KE in MPAS-Ocean:
image

Copy link
Copy Markdown

@hyungyukang hyungyukang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m approving this PR based on the cTests, Polaris tests, and the thorough reviews from the other reviewers. The PR appears to be working as intended. However, the issue we are seeing may be due to the combination of other terms. I think we can move forward with merging this PR into develop and revisit the issue afterward.

@sbrus89, thank you very much for your great work!

@mark-petersen
Copy link
Copy Markdown
Collaborator

Merged locally into develop. Passes all cTests with gnu on perlmutter and Frontier, CPU and GPU on both.

@mark-petersen mark-petersen merged commit 41e74ae into E3SM-Project:develop Apr 6, 2026
1 check passed
@xylar xylar mentioned this pull request Apr 12, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants